API review

Proposer: Josh Faust

Present at review:

• List reviewers

See also:

• (goals) http://www.ros.org/wiki/roscpp/NewMessageAPIProposal

• (proposed implementation) http://www.ros.org/wiki/roscpp/NewMessageAPIProposal2

• (code) https://code.ros.org/svn/ros/stacks/ros/branches/cpp_message_overhaul

  • specifically see message_traits.h, serialization.h and service_traits.h in roslib

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Eric

I had a number of questions, but they all got answered by the time I got to the end. This looks really nice and well thought-out to me.

Thoughts on the questions:

1. SubscriptionMessageHelper doesn't seem like a uniquely good name. I would go ahead and implement the new ones with a different name, and deprecate the old versions, since that will let people play with the new system without breaking client libraries.

2. Since we currently don't have any need for it, I'd push that off. Anyone who really needs it can write a custom serializer.
3. default or standard serializer?
4. For use in realtime that would definitely be nice to be able to do. Are boost shared pointers realtime safe?
   • [Josh]: The reference count is done atomically, so copying them is realtime safe. Their creation can be made realtime safe as well, since they support allocators
5. No opinion

Brian

Like Eric, I had questions along the way, but they were pretty much all answered. Some that weren't:

• Is there value in implementing the << and >> operators for the IStream and OStream classes?

  • [Josh]: Since it's trivial to do, I see no reason not to

• How is LStream used?

  • [Josh]: LStream really only exists because of the allinone serializer

• I assume that the _foo_type typedefs (e.g., _frame_id_type) are intended to allow introspection; have you tried using them, to make sure you have everything you'll need?

  • [Josh]: These have been around for a while (were requested to make actionlib require fewer template arguments)

On the questions:

1. Agreed with Eric, if it's doable.
2. Agreed with Eric.

3. twoway, symmetric, bidirectional?

4. If I template on an allocator type, I'd expect it to be used when I call or new or delete.

5. The implemented solution looks good to me, but the use of event isn't obvious. What about adding a connection header turns a message into an event?

Josh

• I'd like to change the Allocator template name to ContainerAllocator, and not deal with overriding new/delete or providing an allocation function. STL allocators are really meant for containers and it's not possible to correctly implement delete[] using them.

• I'd prefer to use a concrete type as the allocator template parameter, and use rebind to switch to other types

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

• On the question of SubscriptionMessageHelper: rename it SubscriptionCallbackHelper, and fix users of it with conditional compilation. Don't support backward compatibility in roscpp, because it's not worth it.

  • DONE

• allinone -> allInOne

  • DONE

• Stream operators will be defined for !IStream and !OStream

  • DONE

• Hide details of !LStream and make serializedLength() return a uint32

  • DONE

• Leave MessageEvent as is; include message receipt time to metadata

  • DONE

• Change Allocator to ContainerAllocator, and add constructor that accepts reference to a ContainerAllocator.

  • DONE

• Go ahead with concrete Allocator type, and use rebind to switch between types

  • DONE